Add VectorTilesRasterOverlay#1365
Conversation
|
Hey @kring! Are you able to take a glance from an architectural level for this PR? |
Problem is that the dataset I'm using has a bunch of polygons in one primitive...
|
If I can try to summarize how this works... Every time we need a raster image for a terrain tile, we call
The futures returned by The This is potentially a lot of traversals. For vector tilesets with a small number of tiles, it probably isn't too bad. But I think it'll get pretty expensive for bigger ones. Another more subtle problem is that a single call to I don't have any easy solution to this. Probably the ideal approach is more polling-based and incremental. That is, perhaps So I'd say it's reasonable to move forward with the current approach, and just keep an eye on how much time we're spending in this process as users start to work with larger vector tilesets. |
| // part 1 - selecting from scratch | ||
| if (this->_pTilesetContentManager->getRootTile() == nullptr) { | ||
| return this->_pTilesetContentManager->getRootTileAvailableEvent() | ||
| .thenInWorkerThread([this, |
There was a problem hiding this comment.
Two problems here:
- I think the root tile available event can resolve while the root tile is still nullptr, e.g., if there's an error. If it does, this will be an endless promise cycle.
- It's not valid to call
loadTileImageFromTilesetin a worker thread, because it accesses theTilesetContentManager.
There was a problem hiding this comment.
If the root tile is still nullptr by the time it gets to findAndLoadTiles, it will return an empty set of tiles and so return an empty image. I don't think there's a way for it to get stuck in a loop.
|
@kring just optionally requesting a review if you want to take another look. I'm doing a pass myself anyways :) |
| * This may be `nullptr` if there is currently no root tile. | ||
| */ | ||
| Tile* getRootTile() noexcept; | ||
|
|
There was a problem hiding this comment.
Maybe a good idea to discourage consumers of cesium-native from using this... would @private work for our documentation?
There was a problem hiding this comment.
Same for updateTileContent - I think it's fine to leave it open.
| * | ||
| * @param tile The tile to update. | ||
| */ | ||
| void updateTileContent(Tile& tile); |
There was a problem hiding this comment.
I think it's perfectly reasonable that a cesium-native consumer might want to use loadTiles directly and would need a way to access and modify the tiles and to update the tile content. I think it's fine to leave public with the explanation that it does not normally need to be called.
3 participants
This PR adds support for rendering vector tilesets as a raster overlay. It uses a rasterization approach similar to the existing
GeoJsonDocumentRasterOverlay, but instead of working from a single document, it works from a 3D Tiles tileset.Future work after this PR is merged
GeoJsonDocumentRasterOverlaywe got away with letting users customize the styles on elements directly in the document before the raster overlay was loaded, so users can accomplish any kind of metadata styling or other sort of styling they're looking for. But we'll need a different approach for a 3D Tiles tileset. Is it finally time to handle style expressions? (Support for 3D Tiles Styling language #1208)EXT_mesh_polygonextension (see Add EXT_mesh_polygon KhronosGroup/glTF#2570) will continue to evolve, possibly in ways that break the current implementation of this extension. We will need to keep up-to-date with the changes and implement them as necessary.3DTILES_content_gltf_vector(Add3DTILES_content_gltf_vector3d-tiles#838) as it matures.